Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: clippy fixes #728

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Mar 15, 2023

What

Add a clippy check to CI and make initial pass at fixing clippy errors.

At first the number of lints was way too high to tackle all at once. However, disabling some that could be debatable allowed a lot of the obvious fixes to be done.

In future PRs the disabled lints can be addressed in batches to ensure so that it's not too overwhelming.

Why

Currently the only repo that requires that clippy is the CLI and this repo seems more important to have standard coding practices. Clippy can even help catch some logic errors.

Known limitations

[TODO or N/A]

@willemneal willemneal requested review from graydon, sisuresh and a team as code owners March 15, 2023 17:46
@willemneal
Copy link
Member Author

One other thing to note. Currently it should fail when run for the first time. The reason is that the latest commit to main added two methods which return a result, but will never return an error. I kept it in partly as a check that CI should fail the first time and unlike the initial batch of changes it is an API change to remove the Result.

@willemneal
Copy link
Member Author

The CI failed as expected an I disabled unnecessary_wraps for now so it should pass now.

Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this cleanup. This looks good to me for the most part, besides a couple questionable diagnostics/changes I've pointed in comments.

soroban-env-common/src/env_val.rs Show resolved Hide resolved
soroban-env-host/src/auth.rs Show resolved Hide resolved
soroban-env-host/src/host.rs Show resolved Hide resolved
soroban-test-wasms/src/lib.rs Show resolved Hide resolved
soroban-synth-wasm/src/mod_emitter.rs Outdated Show resolved Hide resolved
@dmkozh
Copy link
Contributor

dmkozh commented Mar 16, 2023

The change looks good to me - I think we can merge it when the fmt check is passed and the remaining few failures are resolved.

@dmkozh
Copy link
Contributor

dmkozh commented Mar 20, 2023

CI fails with error: rustup is not installed at '/home/runner/work/rs-soroban-env/rs-soroban-env/.cargo/ci/config.toml'

I'm not sure I understand how to fix it, but I suppose the reason has something to do with the newly introduced config.toml

@willemneal
Copy link
Member Author

@dmkozh fixed it. Was an ENV VAR for cargo home, which was from a previous attempt to have the clippy lints in a file.

@dmkozh
Copy link
Contributor

dmkozh commented Mar 20, 2023

Seems like there are some failures in guest library.

@willemneal
Copy link
Member Author

@dmkozh I also seem to get the following error if I ran that command locally:

cargo clippy --locked --target wasm32-unknown-unknown --no-default-features

I get:

error[E0152]: found duplicate lang item `panic_impl`
  --> soroban-env-common/tests/no_std/src/lib.rs:17:1
   |
17 | fn handle_panic(_: &core::panic::PanicInfo) -> ! {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the lang item is first defined in crate `std` (which `soroban_env_common` depends on)
   = note: first definition in `std` loaded from /Users/willem/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/lib/libstd-a9c931054ca2959f.rlib
   = note: second definition in the local crate (`test_no_std`)

For more information about this error, try `rustc --explain E0152`.

@dmkozh
Copy link
Contributor

dmkozh commented Mar 20, 2023

@dmkozh I also seem to get the following error if I ran that command locally:

cargo clippy --locked --target wasm32-unknown-unknown --no-default-features

I get:

error[E0152]: found duplicate lang item `panic_impl`
  --> soroban-env-common/tests/no_std/src/lib.rs:17:1
   |
17 | fn handle_panic(_: &core::panic::PanicInfo) -> ! {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the lang item is first defined in crate `std` (which `soroban_env_common` depends on)
   = note: first definition in `std` loaded from /Users/willem/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/wasm32-unknown-unknown/lib/libstd-a9c931054ca2959f.rlib
   = note: second definition in the local crate (`test_no_std`)

For more information about this error, try `rustc --explain E0152`.

That's likely because you're missing --features testutils (probably that should be fixed separately...)

@graydon
Copy link
Contributor

graydon commented May 4, 2023

Hey @willemneal sorry for the delay (and inevitable bitrot).

I've merged a run of cargo clippy --fix in #790 and turned on clippy in CI without the style group in #796. So that accomplishes maybe half or a third of what this PR was trying to do, but lays groundwork. Assuming you want to enable all of the extras (or even all the way to enabling the full style group) I'd recommend refreshing this PR in stages -- doing a handful of new lints at a time, since it's easier to review a small or medium-sized PR than a giant one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants